Skip to content

Conversation

fredden
Copy link

@fredden fredden commented Aug 21, 2023

I noticed this bug while working on another contribution. After calling opendir(), the first two results are thrown away. While these are usually . and .., that's not guaranteed. In my case, the language directory in question was one of these first two entries, so no documentation was generated. The first change here is no longer throwing away the first two directory entries. The second change, is to ignore all entries which have a dot in their name. No language codes include a dot. (Some have a dash/hyphen/underscore, but no dots.) This realisation lead to cleaner code and more efficient run as we no longer have to ask the file-system if something is a directory or not as often.

@fredden

This comment was marked as outdated.

@fredden fredden force-pushed the dir-order-not-guaranteed branch from 146369c to c7782d0 Compare August 21, 2023 17:24
@derickr
Copy link
Member

derickr commented Jul 30, 2024

Is this still relevant?

@fredden
Copy link
Author

fredden commented Jul 31, 2024

Is this still relevant?

Yes, I think it is. The bug still exists. However, it's likely that the system on which this runs for production just happens to return the . and .. entries first (by chance), so the bug isn't witnessed there.

continue;
}
if ($f === '.git') {
if (strpos($f, '.') !== false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be simply if ($f[0] === '.'), a . could exist in the reset of the filename.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do this, however a dot is not a valid character in a language code.

@derickr
Copy link
Member

derickr commented Aug 13, 2024

@fredden ?

@fredden
Copy link
Author

fredden commented Aug 13, 2024

@fredden ?

Hi. I'm away from my laptop currently, so I can't easily update the code here. Would you like me to apply the change you suggested? I think the code is fine as-is, but I will make changes as required.

@fredden fredden requested a review from derickr August 22, 2024 21:53
@jimwins
Copy link
Member

jimwins commented Nov 19, 2024

This is a bit of a tangent, but just to throw it out there -- this script should not live in the systems repo, we should move it into doc-base so developing/maintaining it sits with the team of folks working on the documentation tooling.

@derickr
Copy link
Member

derickr commented Dec 12, 2024

@jimwins I am not sure if I agree here, as this is code that runs while doing things with specifically checked out things on the server?

@jimwins
Copy link
Member

jimwins commented Dec 12, 2024

@jimwins I am not sure if I agree here, as this is code that runs while doing things with specifically checked out things on the server?

I hadn't looked at what the script operated on (generated version of the docs), but in general I think this is analogous to the translation status script we moved from web-doc to doc-base. I think that scripts generating information about the documentation should be in doc-base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants